Skip to content

FIX: forked-pr-coverage.yml env-injection via untrusted artifact - #714

Merged
Sumit Sarabhai (sumitmsft) merged 7 commits into
mainfrom
sumitmsft-argus-ado46466
Aug 20, 2026
Merged

FIX: forked-pr-coverage.yml env-injection via untrusted artifact#714
Sumit Sarabhai (sumitmsft) merged 7 commits into
mainfrom
sumitmsft-argus-ado46466

Conversation

@sumitmsft

@sumitmsft Sumit Sarabhai (sumitmsft) commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

AB#46466

Summary

  • Treat fork-produced coverage artifacts as untrusted input and validate their schema and values before use.
  • Bind coverage comments to the triggering pull request instead of accepting an artifact-supplied PR number.
  • Remove untrusted artifact propagation through GITHUB_ENV and reject unexpected files, unsafe URLs, control characters, and oversized values.
  • Pin GitHub Actions to immutable commit SHAs and disable persisted checkout credentials.
  • Add regression coverage for environment injection, cross-PR targeting, executable artifacts, and Markdown/URL injection.

Validation

  • 12 targeted security regression tests passed.
  • Changed workflow YAML parsed successfully.
  • Black formatting checks passed.
  • Native extension build completed successfully.

Validate untrusted coverage artifacts before posting comments, bind comments to the triggering pull request, remove privileged environment propagation, and pin actions to immutable revisions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 11, 2026 11:16
@github-actions github-actions Bot added the pr-size: large Substantial code update label Aug 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the forked PR coverage-comment pipeline by treating coverage artifacts as untrusted input, validating/normalizing the data used to generate PR comments, and reducing the privileged workflow’s exposure to artifact-driven environment injection.

Changes:

  • Introduces a Python validator/comment builder (prepare_fork_coverage_comment.py) and a security-focused regression test suite.
  • Refactors forked-pr-coverage.yml to validate artifacts + resolve the target PR from the triggering workflow context (not artifact-supplied data), and post/update a single sticky comment.
  • Tightens the producer workflow (pr-code-coverage.yml) by removing artifact-supplied PR targeting fields, hardening multiline GITHUB_ENV usage, and pinning actions to SHAs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_fork_coverage_security.py Adds regression tests for artifact schema validation, URL/markup injection, PR resolution, and workflow env-injection checks.
.github/workflows/pr-code-coverage.yml Pins actions, disables persisted credentials, hardens multiline env export, and removes untrusted fields from the uploaded artifact payload.
.github/workflows/forked-pr-coverage.yml Moves privileged workflow to validate downloaded artifacts + resolve PR from the event/commit association before commenting.
.github/scripts/prepare_fork_coverage_comment.py Implements schema/value validation and safe comment construction for fork coverage artifacts.
.github/actions/post-coverage-comment/action.yml Pins the sticky-comment action to an immutable SHA.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/scripts/prepare_fork_coverage_comment.py
Comment thread tests/test_fork_coverage_security.py
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

82%


📈 Total Lines Covered: 7757 out of 9436
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Enforce the one-file artifact schema without recursively traversing attacker-controlled directory trees.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting changes - some issues wrt forked pr comments and refactoring suggestions to cut the PR short

Comment thread .github/scripts/prepare_fork_coverage_comment.py
Comment thread .github/scripts/prepare_fork_coverage_comment.py Outdated
Comment thread .github/workflows/forked-pr-coverage.yml Outdated
Comment thread .github/scripts/prepare_fork_coverage_comment.py Outdated
Comment thread .github/workflows/forked-pr-coverage.yml Outdated
Comment thread .github/scripts/prepare_fork_coverage_comment.py
Comment thread .github/workflows/forked-pr-coverage.yml Outdated
Comment thread .github/scripts/prepare_fork_coverage_comment.py
Comment thread .github/scripts/prepare_fork_coverage_comment.py Outdated
Comment thread .github/workflows/forked-pr-coverage.yml Outdated
- forked-pr-coverage.yml: query /pulls?state=open (paginated) instead of
  /commits/{sha}/pulls, which never returns a fork's head commit, so fork
  PRs never resolved and no coverage comment was ever posted.
- prepare_fork_coverage_comment.py: remove the workflow_run.pull_requests
  fast-path; GitHub always sends it empty for fork runs, so it was dead
  code that only the happy-path test exercised.
- tests: point the happy-path test at the real head-SHA resolution and add
  a regression that an attacker-supplied event pull_requests entry is ignored.
- Revert action SHA pins (checkout, upload-artifact, sticky-comment) to tags
  to avoid colliding with #716, which owns SHA pinning.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- prepare_fork_coverage_comment.py: require the six expected fields to be
  present rather than an exact set, so adding a field to the producer no
  longer silently disables fork coverage comments. All six values are still
  strictly validated and only those six are propagated.
- tests: assert an artifact-supplied pr_number is tolerated but never
  propagated, and that a missing required field is still rejected.
- forked-pr-coverage.yml: drop the RUN_ID, HEAD_SHA and PR_NUMBER shell
  guards. RUN_ID/HEAD_SHA come straight from the trusted workflow_run event
  (HEAD_SHA is re-validated in the script), and PR_NUMBER only re-checks the
  script's own validated output. HEAD_SHA env removed as it is now unused.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting changes - for a different blocker this time, added a couple of suggestions as well

Comment thread .github/workflows/forked-pr-coverage.yml
Comment thread .github/scripts/prepare_fork_coverage_comment.py
Comment thread .github/workflows/forked-pr-coverage.yml
- forked-pr-coverage.yml: gh api rejects --slurp combined with --jq
  ("the --slurp option is not supported with --jq or --template"), which
  failed the whole step. Pipe --slurp output to jq instead, for both the
  open-PR fetch and the coverage-comment lookup. Verified against gh 2.96
  and a real fork PR end to end.
- prepare_fork_coverage_comment.py: also match the triggering fork's head
  repository (from the trusted workflow_run event), so two forks sitting on
  the same commit resolve to the correct PR instead of colliding and
  dropping the comment. Reject events missing the head repository.
- tests: add head-repo disambiguation and missing-head-repository cases;
  factor pull fixtures into a shared helper.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@sumitmsft
Sumit Sarabhai (sumitmsft) merged commit 5f4fd10 into main Aug 20, 2026
29 checks passed
Subrata (subrata-ms) added a commit that referenced this pull request Aug 28, 2026
### Work Item / Issue Reference  
<!-- 
IMPORTANT: Please follow the PR template guidelines below.
For mssql-python maintainers: Insert your ADO Work Item ID below 
For external contributors: Insert Github Issue number below
Only one reference is required - either GitHub issue OR ADO Work Item.
-->

<!-- mssql-python maintainers: ADO Work Item -->
>
[AB#47683](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/47683)

<!-- External contributors: GitHub Issue -->


-------------------------------------------------------------------
### Summary   
<!-- Insert your summary of changes below. Minimum 10 characters
required. -->
**Symptom**
Build-Release-Package-Pipeline failed on all four Linux variants during
Step 8 (pytest) with a collection-time error, before any test ran:
tests/test_fork_coverage_security.py:10: in <module>
    SPEC.loader.exec_module(coverage_comment)
E   FileNotFoundError: [Errno 2] No such file or directory:
'/test_isolated_cp310/.github/scripts/prepare_fork_coverage_comment.py'
!!! Interrupted: 1 error during collection !!!

Combined with --maxfail=1, this aborted cp310–cp314 on:

- Linux manylinux_2_28 x86_64
- Linux manylinux_2_28 aarch64
- Linux musllinux x86_64
- Linux musllinux aarch64

**Root cause**
Two independent design assumptions collided:

- Test-side assumption. [test_fork_coverage_security.py:7] resolves
helpers at module top level (collection time) via
[Path(__file__).parents[1] / ".github" / "scripts" /
"prepare_fork_coverage_comment.py"], and also reads
[forked-pr-coverage.yml] and [pr-code-coverage.yml]. It assumes
[parents[1]] is a full repo checkout.

Pipeline-side assumption. The Linux release lane in
[build-linux-single-stage.yml] intentionally runs pytest from an empty
isolated directory to prove the installed wheel is self-contained:

TEST_DIR="/test_isolated_${PYBIN}"
rm -rf $TEST_DIR; mkdir -p $TEST_DIR
$PY -m pip install -q "$WHEEL"
cp -r /workspace/tests $TEST_DIR/
cp /workspace/pytest.ini $TEST_DIR/
cp /workspace/requirements.txt $TEST_DIR/

Only [tests], [pytest.ini], [requirements.txt] are copied. [.github] is
not — until now no test needed anything outside [tests].
[test_fork_coverage_security.py] (added by the fork-coverage hardening
series culminating in PR #714) is the first test that reaches outside
[tests] at import time. In the isolated layout [parents[1]]=
/test_isolated_cp310/, so the .github/… lookup fell off the map. Only
the Linux lane runs the empty-isolated-dir model; Windows and macOS run
from $(Build.SourcesDirectory) (a real checkout), so they were
unaffected — and repo-root GitHub CI was unaffected too.

**Fix:**
Make the pipeline meet the test's assumption, and drop an earlier
stop-gap.

Change added to Linux release pipeline
Copy [.github] into $TEST_DIR alongside [tests], in both the manylinux
branch (covers x86_64 + aarch64) and the musllinux branch (covers x86_64
+ aarch64):

[build-linux-single-stage.yml:322] — manylinux bash -lc block:

[build-linux-single-stage.yml:392] — musllinux sh -lc block: same two
lines.

cp -r /workspace/tests $TEST_DIR/ || echo "WARNING: No tests directory";
Some tests read repo-side helper scripts/workflows (e.g.
.github/scripts/prepare_fork_coverage_comment.py).
cp -r /workspace/.github $TEST_DIR/ || echo "WARNING: No .github
directory";

These are the only two code sites reached by the four failing Linux
stages.
<!-- 
### PR Title Guide

> For feature requests
FEAT: (short-description)

> For non-feature requests like test case updates, config updates ,
dependency updates etc
CHORE: (short-description) 

> For Fix requests
FIX: (short-description)

> For doc update requests 
DOC: (short-description)

> For Formatting, indentation, or styling update
STYLE: (short-description)

> For Refactor, without any feature changes
REFACTOR: (short-description)

> For performance improvements
PERF: (short-description)

> For release related changes, without any feature changes
RELEASE: #<RELEASE_VERSION> (short-description) 

### Contribution Guidelines

External contributors:
- Create a GitHub issue first:
https://github.com/microsoft/mssql-python/issues/new
- Link the GitHub issue in the "GitHub Issue" section above
- Follow the PR title format and provide a meaningful summary

mssql-python maintainers:
- Create an ADO Work Item following internal processes
- Link the ADO Work Item in the "ADO Work Item" section above  
- Follow the PR title format and provide a meaningful summary
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants